feat(studio): compact latency duration format across experiments and intake - #632
Merged
Conversation
…riments and intake
Latency was formatted three different ways: experiment surfaces inlined
`${Math.round(ms)} ms`, intake used `formatDurationMs` (ms/s/min, 2 decimals),
producing long millisecond numbers in the experiment group list, experiment
detail header, and session/task tables.
Add a shared `formatDuration` util that renders compact segments
(`10m 12s 13ms`, `12s 34ms`, `34ms`) — highest non-zero unit down to the lowest,
interior zeros kept, hours supported, sub-ms precision preserved. Point the
experiment surfaces at it and alias `formatDurationMs` to it so all intake
trace/span surfaces pick up the new format with no call-site churn. Drop the
whitespace-strip in SpanTriggerMeta that would collapse the segmented format.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA shared ChangesDuration Formatting
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
walston
requested changes
Jul 10, 2026
walston
left a comment
walston
left a comment
Contributor
There was a problem hiding this comment.
Open to pushback, just don't want parallel solutions to the same problem sprinkled throughout the codebase.
…urationMs Address review: formatTimeInSeconds already lived in @nemo/common/src/utils/date and did the same segmented formatting. Instead of a parallel studio-local formatDuration, add an ms-aware formatDurationMs primitive next to it and make formatTimeInSeconds delegate to it, so there is a single segmentation implementation. formatDurationMs drops zero-valued units (including interior ones, e.g. `1h 5s`) to match formatTimeInSeconds, keeps sub-ms precision, and adds an `ms` component (`10m 12s 13ms`, `12s 34ms`, `34ms`). Repoint the experiment surfaces and intakeTelemetry's re-export at the common helper and delete the studio-local util. formatTimeInSeconds behavior is unchanged (verified by its existing tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
…ormat The trace headline-metrics test asserted the old `12.23 s` format; the shared formatDurationMs now renders `12s 230ms`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
walston
approved these changes
Jul 13, 2026
walston
reviewed
Jul 13, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
web/packages/common/src/utils/date.test.ts (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelative import violates the no-relative-import-paths guideline.
from './date'is a relative import to a sibling file. As per path instructions, "Never use relative imports. Always use absolute alias paths ... even for sibling files in the same directory. ESLint enforces this withno-relative-import-paths."♻️ Proposed fix
-import { formatDurationMs, formatTimeInSeconds, utcToLocalDate } from './date'; +import { formatDurationMs, formatTimeInSeconds, utcToLocalDate } from '`@nemo/common/src/utils/date`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/utils/date.test.ts` at line 4, Update the date utility import in the test to use the repository’s configured absolute alias path instead of the relative './date' path, while preserving the existing imported symbols formatDurationMs, formatTimeInSeconds, and utcToLocalDate.Source: Path instructions
web/packages/common/src/utils/date.ts (1)
77-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing explicit return type on public API.
formatDurationMsdeclares: string, butformatTimeInSecondsdoesn't. As per coding guidelines, "Use explicit return types for public APIs and complex functions in TypeScript."♻️ Proposed fix
-export const formatTimeInSeconds = (seconds?: number) => { +export const formatTimeInSeconds = (seconds?: number): string => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/packages/common/src/utils/date.ts` at line 77, Update the public function formatTimeInSeconds to declare its explicit return type as string, matching the existing formatDurationMs API style and coding guidelines.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/packages/common/src/utils/date.ts`:
- Around line 77-81: Update formatTimeInSeconds to return an empty string when
seconds is less than 1, before applying Math.floor or calling formatDurationMs.
Preserve the existing empty handling for undefined and zero, and continue
formatting whole-second values unchanged.
---
Nitpick comments:
In `@web/packages/common/src/utils/date.test.ts`:
- Line 4: Update the date utility import in the test to use the repository’s
configured absolute alias path instead of the relative './date' path, while
preserving the existing imported symbols formatDurationMs, formatTimeInSeconds,
and utcToLocalDate.
In `@web/packages/common/src/utils/date.ts`:
- Line 77: Update the public function formatTimeInSeconds to declare its
explicit return type as string, matching the existing formatDurationMs API style
and coding guidelines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f69f43ba-574f-4af6-b4bb-432bd645023a
📒 Files selected for processing (7)
web/packages/common/src/utils/date.test.tsweb/packages/common/src/utils/date.tsweb/packages/studio/src/components/IntakeDetail/IntakeComponents/traceKeyValues.test.tsxweb/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsxweb/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsxweb/packages/studio/src/routes/ExperimentDetailRoute/ExperimentDetailMetrics.tsxweb/packages/studio/src/util/intakeTelemetry.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- web/packages/studio/src/components/dataViews/ExperimentSessionsDataView/index.tsx
- web/packages/studio/src/components/dataViews/ExperimentGroupDataView/index.tsx
…ds sub-second empty Address review: - ExperimentDetailMetrics: remove the `!= null` guard before formatDurationMs. formatDurationMs already returns '—' for null/undefined, which is also KVPair's default empty value, so the guard was pure redundancy with identical output. - formatTimeInSeconds: guard `seconds < 1` so fractional-second inputs render '' as before, instead of '0ms' (Math.floor(seconds) * 1000 collapsed to 0). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
…on-format/rrhyne Signed-off-by: Rob Rhyne <rrhyne@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Latency was rendered as long raw-millisecond numbers and formatted three different ways across the app. This consolidates all latency/duration rendering onto one shared formatter that produces compact segments:
10m 12s 13ms12s 34ms34msWhy
Before, three formatters coexisted:
`${Math.round(ms)} ms`— long unscaled milliseconds.formatDurationMs(ms/s/min, 2 decimals).Changes
web/packages/studio/src/util/duration.ts—formatDuration(ms): renders highest non-zero unit down to lowest, drops leading/trailing zero units, keeps interior zeros (1h 0m 5s), supports hours, preserves sub-ms precision (0.34ms), returns—for null/undefined.intakeTelemetry.ts—formatDurationMsnow aliasesformatDuration, so all six intake trace/span surfaces adopt the new format with zero call-site churn.ExperimentGroupDataView,ExperimentDetailMetrics,ExperimentSessionsDataView— callformatDurationinstead of inline millisecond strings.SpanTriggerMeta.tsx— removed a.replace(/\s+/g,'')that would collapse the segmented format into10m12s13ms.Testing
duration.test.ts— 10/10 pass (covers the examples above plus edges: hours, interior zeros, rounding, sub-ms, zero/negative).SpanEvaluationContexterrors inintakeTelemetry.tsare pre-existing onmainfrom an SDK schema change, unrelated to this PR).Out of scope
The Agent Monitor inference-logs table has its own route-local
formatDuration(ms/s/min, 1 decimal); left untouched intentionally.🤖 Generated with Claude Code
Summary by CodeRabbit